-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Whois component maintenance, PR 1 of 3: Light refactoring, adding abstraction #117749
Whois component maintenance, PR 1 of 3: Light refactoring, adding abstraction #117749
Conversation
Add a thin abstraction layer to make it easy to replace the external whois-query library
Hey there @frenck, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependency update: Replacement of unmaintained whois upstream library with a maintained one
Which one are you planning to replace it with? It seems like the package maintenance simply moved into another repo/package?
There is mixed content in this PR IMHO. One is the refactoring of adding abstraction and some test refactoring as well as it seems.
I'm not in favor of the added abstraction. The attracting is what we use the Python packages for, there is no need to add another abstraction to this all (it will only add complexity), nor is it a goal to make packages easily swappable in Home Assistant.
../Frenck
@@ -0,0 +1,64 @@ | |||
"""A helper class that abstracts away the usage of external whois libraries.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an uncommon level of abstraction for Home Assistant. This is basically why we use libraries.
|
||
|
||
@dataclass(kw_only=True) | ||
class Domain: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be typing from upstream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at the final code after all 3 planned PRs listed in the description. I believe it will help you understand why I decided to do it with a helper, instead of putting all the processing logic directly in HA classes.
class WhoisUnknownTLD(Exception): | ||
"""Exception class when unknown TLD encountered.""" | ||
|
||
|
||
class GenericWhoisError(Exception): | ||
"""Exception class for all other exceptions originating from the external whois library.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should come from upstream
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
The replacement recommended by the maintainer of the current library is Ultimately, I quickly evaluated two replacement candidates:
If you want to see the full scope of my changes, here is the complete branch that includes all changes I currently split into 3 parts listed in the PR description.
IMHO, it still should be considered as one logical change - modifications to tests were needed as part of modifying code they were testing.
It seemed unnecessary at first to me as well, because the current library has a decent and reliable interface. |
This integration migrated away from that library because of many problems. Seems odd to revert that.
The point here is, they could be separate, and thus should be in a separate PR to make PR as small as possible.
Which is why it was removed.
Sorry, as per review comment, we are not in agreement. |
Okay, I'm definitely open to that, but first, we have to agree on the approach to the unmaintained and broken whois library first
Okay, I would appreciate some constructive feedback then. HA uses a broken whois library and the recommended replacement is broken as well (talking about My proposed replacement library is far from perfect, but it seems to be working better for the TLDs I tested (including com, net, org, io, me, us, pl...). The choice is between two libraries that have their issues. What do you suggest we do? |
I'm not sure if it is broken, this PR doesn't reference anything that indicates that?
Please remove the additional layer of abstraction... 🤷 |
I listed some issues in #117805.
This particular Pull Request is essentially for adding this layer of abstraction. If it's not acceptable for Home Assistant, this PR should be wholly rejected and closed. Side note: rejecting this PR would render the rest of my work on the Whois integration (https://github.com/mkrasowski/homeassistant-core/tree/replace-unmaintained-whois-library) inapplicable, because it's built on top of this abstraction. |
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Add a thin abstraction layer to make it easy to replace the external whois-query library
Proposed change
This is Pull Request 1 of 3 that I have lined up as part of my contributions to "whois" component upkeep.
Order of changes:
Originally it was one larger PR, but I decided to split it up following the perfect PR recommendations.
Ideally, I'd like to get these 3 PRs merged in a relatively quick succession.
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: